[PROPOSAL] Switch to pytest style test classes, use plain asserts #4204

epwalsh · 2020-05-07T18:07:24Z

As far as I can tell, since we're using pytest to run tests, there's no benefit of using unittest.TestCase as the base class for AllenNlpTestCase as opposed to plain pytest style classes.

There is, however, several disadvantages:

@pytest.mark.parametrize does not work with methods on a unittest.TestCase class. Given that parametrizing tests is such a powerful tool, I think this is a major issue.
unittest.TestCase assertion methods (self.assertEqual, self.assertSetEqual, etc) are less concise than plain assert statements, which are recommended by pytest.
The setUp and tearDown methods do not have PEP-8 compatible names, and it's not clear when these methods are run based on their names. On the other hand, the pytest equivalents (setup_method and teardown_method or setup_class and teardown_class) have snake-case names that are perfectly clear as to when they run.

I know this looks like a huge PR but it's mostly just replacing unittest assertion methods with the pytest equivalents.

epwalsh · 2020-05-07T18:14:25Z

The "Check Models" workflow will fail until the unittest assertion methods in the models repo are replaced with the pytest equivalents.

epwalsh · 2020-05-07T18:20:25Z

allennlp/common/testing/test_case.py

 _available_devices = ["cpu"] + (["cuda"] if torch.cuda.is_available() else [])
-multi_device = parametrize(("device",), [(device,) for device in _available_devices])
+
+multi_device = lambda f: pytest.mark.parametrize("device", _available_devices)(pytest.mark.gpu(f))


The pytest.mark.gpu(f) ensures these tests are ran in the "GPU Checks" workflow

dirkgr · 2020-05-07T18:35:12Z

I have been wanting to do this for a while. It's especially confusing in the models repo, because AllenNlpTestCase supplies some functions that don't work there.

dirkgr

This is a bit different than what I thought you were doing. I thought you were changing everything to top-level functions named test_*, with common setup and tear-down in pytest fixtures. The base class AllenNlpTestCase would then go away. Any reason not to do that?

dirkgr · 2020-05-07T18:36:15Z

allennlp/tests/commands/docstring_help_test.py

-                self.assertEqual(
-                    expected_output,
-                    actual_output,
+                assert expected_output == actual_output, (


Will this give me those pretty comparisons, that show immediately where the difference is, even in large data?

epwalsh · 2020-05-07T18:47:04Z

This is a bit different than what I thought you were doing. I thought you were changing everything to top-level functions named test_*, with common setup and tear-down in pytest fixtures. The base class AllenNlpTestCase would then go away. Any reason not to do that?

I think having a base class is beneficial because it wraps up all of its fixtures. Without it, you would have to import the fixtures (the temp directory and whatever else the base class comes with) to each test module or to a conftest.py at the same directory level as the test module, because there is no way of declaring global fixtures that work for all test modules without explicitly importing them.

That said, if you don't need those fixtures then it's just unnecessary overhead. So we could probably just use def test_* functions in many places.

epwalsh · 2020-05-07T18:52:51Z

.github/workflows/pull_request.yml

@@ -116,7 +116,9 @@ jobs:
        ALLENNLP_VERSION_OVERRIDE: ""  # Don't replace the core library.
      run: |
        git clone https://github.com/allenai/allennlp-models.git
-        cd allennlp-models && pip install --upgrade --upgrade-strategy eager -e . -r dev-requirements.txt
+        cd allennlp-models
+        git checkout test-case  # TODO remove this line


Just putting this here temporarily to make sure the models tests pass on that branch

matt-gardner

LGTM! I would vote strongly against moving everything to top-level test methods, by the way. In addition to what @epwalsh said, it makes model tests more annoying.

matt-gardner · 2020-05-07T20:02:52Z

allennlp/tests/data/fields/sequence_label_field_test.py

@@ -65,25 +66,25 @@ def test_sequence_label_field_raises_on_incorrect_type(self):
        with pytest.raises(ConfigurationError):
            _ = SequenceLabelField([[], [], [], [], []], self.text)

-    def test_class_variables_for_namespace_warnings_work_correctly(self):
+    def test_class_variables_for_namespace_warnings_work_correctly(self, caplog):


Nit: caplog is not a great variable name.

This is actually an out-of-the-box pytest fixture so unfortunately I think we're stuck with that name

https://docs.pytest.org/en/latest/logging.html#caplog-fixture

Oh, hmm, didn't realize that pytest did this magic. Ok 🤷‍♂️

matt-gardner · 2020-05-07T20:04:19Z

allennlp/tests/training/learning_rate_schedulers/learning_rate_scheduler_test.py

        self.model = torch.nn.Sequential(torch.nn.Linear(10, 10))

    def test_reduce_on_plateau_error_throw_when_no_metrics_exist(self):
-        with self.assertRaises(ConfigurationError) as context:
+        with pytest.raises(ConfigurationError) as context:


I think this should just be something like match="learning rate ...", instead of as context, right?

Good point. Just updated

schmmd

Looks reasonable to me!

switch to pytest style test classes

2fa1470

epwalsh requested review from dirkgr, schmmd and matt-gardner May 7, 2020 18:07

epwalsh added 2 commits May 7, 2020 11:16

fix failing test

9a3643e

ensure multi_device tests are also gpu marked

2916053

epwalsh commented May 7, 2020

View reviewed changes

make flake8 happy

759917b

dirkgr reviewed May 7, 2020

View reviewed changes

epwalsh mentioned this pull request May 7, 2020

update for new AllenNlpTestCase allenai/allennlp-models#45

Merged

temporarily point to different models branch

d2f41f9

epwalsh commented May 7, 2020

View reviewed changes

matt-gardner approved these changes May 7, 2020

View reviewed changes

dirkgr approved these changes May 7, 2020

View reviewed changes

epwalsh added 3 commits May 7, 2020 13:39

matts comment

acb75b0

fix

9e57922

Merge branch 'master' into test-case

0b60169

schmmd approved these changes May 7, 2020

View reviewed changes

epwalsh added 2 commits May 7, 2020 14:04

revert temp models install patch

ecb59e3

Merge branch 'test-case' of github.com:epwalsh/allennlp into test-case

cda126c

epwalsh merged commit 82bf58a into allenai:master May 7, 2020

epwalsh deleted the test-case branch May 7, 2020 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PROPOSAL] Switch to pytest style test classes, use plain asserts #4204

[PROPOSAL] Switch to pytest style test classes, use plain asserts #4204

epwalsh commented May 7, 2020

epwalsh commented May 7, 2020

epwalsh May 7, 2020

dirkgr commented May 7, 2020

dirkgr left a comment

dirkgr May 7, 2020

epwalsh May 7, 2020

epwalsh commented May 7, 2020 •

edited

Loading

epwalsh May 7, 2020

matt-gardner left a comment

matt-gardner May 7, 2020

epwalsh May 7, 2020 •

edited

Loading

epwalsh May 7, 2020

matt-gardner May 7, 2020

matt-gardner May 7, 2020

epwalsh May 7, 2020

schmmd left a comment

[PROPOSAL] Switch to pytest style test classes, use plain asserts #4204

[PROPOSAL] Switch to pytest style test classes, use plain asserts #4204

Conversation

epwalsh commented May 7, 2020

epwalsh commented May 7, 2020

Choose a reason for hiding this comment

dirkgr commented May 7, 2020

dirkgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epwalsh commented May 7, 2020 • edited Loading

Choose a reason for hiding this comment

matt-gardner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epwalsh May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmmd left a comment

Choose a reason for hiding this comment

epwalsh commented May 7, 2020 •

edited

Loading

epwalsh May 7, 2020 •

edited

Loading